I think this looks good 👍 @justindbaur?

This will make it harder/impossible to make a scoped variation. Part of the point of having the HttpClient be able to be per-configured is so others, mostly us, can add the ApiSecret on a per call basis based on a tenant. I'm fine with the other constructors for people without DI but when we have DI we should try and keep with the patterns exposed. We can make the constructor that expects the pre-configured HttpClient internal or [EditorBrowsable(Never)] (which does work in VSCode) to discourage it's use.

This will make it harder/impossible to make a scoped variation.

Can you please share more details? I'm not entirely sure where the issue would arise.

This will make it harder/impossible to make a scoped variation.

Can you please share more details? I'm not entirely sure where the issue would arise.

• https://github.com/passwordless/passwordless-server/blob/main/src/AdminConsole/Services/ScopedPasswordlessClient.cs

But IPasswordlessClient is also used in the ASP.NET Identity SDK, so by default injects the API secret from the options pattern (appsettings.json).

When you're viewing an application in the AdminConsole, we call the back-end with API secrets from that specific application, so we need to override the secret. So that is what ScopedPasswordlessClient is doing.

But debug through it when you're using the AdminConsole.

We should probably come up with a solution where both use a different IHttpClientFactory implementation. Either using named or typed. And then do all the magic in DelegatingHandlers as it allows more flexibility to come up with different solutions such as what @justindbaur talks about.

@jonashendrickx @justindbaur how about this?

public class PasswordlessClientScope
{
    public PasswordlessClient Client { get; }

    public PasswordlessClientScope(HttpClient http, ICurrentContext context)
    {
        Client = new PasswordlessClient(
            http,
            new PasswordlessOptions(context.ApiKey, context.ApiSecret)
        );
    }
}

You can make it implement IPasswordlessClient if you really want (through delegation), but the idea is that you just compose the client by providing the correct options.

Or, the direct equivalent, if you want:

public class ScopedPasswordlessClient : PasswordlessClient
{
    public ScopedPasswordlessClient(HttpClient http, ICurrentContext context)
        : base(http, new PasswordlessOptions(context.ApiKey, context.ApiSecret)
    {
    }
}

@jonashendrickx @justindbaur how about this?

FWIW I think that looks less brittle than https://github.com/passwordless/passwordless-server/blob/main/src/AdminConsole/Services/ScopedPasswordlessClient.cs

Yeah apologies, we can make this work with the scoped version it would just work differently than the original design of using a DelegatingHandler that I think @jonashendrickx had prototyped that I quite liked.

IMO it's okay to trust that you are given objects that will do what they are built to do. Currently we abide by the typed client pattern because this library will primarily be used through DI in ASP.NET Core and this is the kind of stuff that pattern encourages. We also break the D in SOLID by creating a concretion in the constructor but we have already determined we are going to want to test this against a mock server so that matters a bit less.

You all have the majority and will be working on this much more than I will be in the future, feel free to override me.

@justindbaur You're not actively against this or consider the change harmful, right?

As in we're discussing different tastes of the recipe, not adding arsenic to the meal?

As long as there is no veto, I think this rework can go through, from my pov.

Yeah apologies, we can make this work with the scoped version it would just work differently than the original design of using a DelegatingHandler that I think @jonashendrickx had prototyped that I quite liked.

I have unfortunately not seen that prototype.

IMO it's okay to trust that you are given objects that will do what they are built to do

The problem is that HttpClient wasn't built to specialize on accessing the Passwordless API. You can create a generally valid HttpClient that won't be a valid client for the API. On the other hand, with these changes, you cannot create a PasswordlessClient that won't be a valid client for the API -- it will always be valid. So this guarantees trust through the type system.

We also break the D in SOLID by creating a concretion in the constructor but we have already determined we are going to want to test this against a mock server so that matters a bit less.

That doesn't matter either way because we can still inject a fake HTTP handler inside the pipeline and substitute requests however we want to. So the concretion of PasswordlessClient is not important in that regard.

No, this doesn't do any of the things I would reject it for which are

• Creating/disposing of the HttpMessageHandler that holds the connection pool cache.
• Not being able to use the default handler pipeline provided via IHttpClientFactory
• Make us not be able to use the System.Net.Http.Json helpers.

wasn't built

I'm still fine with making the constructor that does expect an HttpClient that is built for it less accessible. The current usage of that constructor does use one that was built for it.

it will always be valid

That's not entirely true, it would still be possible for someone to create an HttpClient that is not compatible with our API. It could be disposed already, it could have to low of a timeout, this makes it harder to mess up but so does slapping [EditorBrowsable(Never)] on the constructor while also keeping us from having to create two HttpClient's for all requests while we can have confidence that the one we get is built for us. Users will get an error on their very first request if they use it incorrectly.

That's not entirely true, it would still be possible for someone to create an HttpClient that is not compatible with our API. It could be disposed already, it could have to low of a timeout, this makes it harder to mess up but so does slapping [EditorBrowsable(Never)] on the constructor while also keeping us from having to create two HttpClient's for all requests while we can have confidence that the one we get is built for us. Users will get an error on their very first request if they use it incorrectly.

Yeah, those are valid points, but I would argue that timeout/lifetime management are infrastructural/x-cutting concerns, while ApiSecret/BaseURL/ProblemDetails are integral to the business logic.

What I'm trying to say is that if you don't set ApiSecret, it will definitely not work. But there's no specific value of a timeout that we require to access the API. If it were specified in the API, for example, that we need to have the timeout of at least 5 seconds, then we could also make that an invariant. But this was also not enforced in the previous approach.

Ultimately the difference in the two approaches comes down to this:

• New approach has a more encapsulated abstraction in terms of PasswordlessClient -- all essential configuration is set up in one place. The old approach relied on consumers (albeit trusted ones) to set up the configuration themselves -- in several separate places.
• New approach has the overhead of an extra HttpClient that just delegates, through a custom handler (that was already used before this PR), requests to the factory-provided HttpClient. However, it also removes the overhead of extra service registrations. Which one is more impactful -- I don't know.

In any case, I appreciate everyone for challenging me and all the feedback on the PR 🙂